fix(code): prevent archived tasks from reappearing after archive#2211
Open
richardsolomou wants to merge 2 commits into
Open
fix(code): prevent archived tasks from reappearing after archive#2211richardsolomou wants to merge 2 commits into
richardsolomou wants to merge 2 commits into
Conversation
archiveTaskImperative wrote optimistic updates to the archive queries without cancelling in-flight refetches first. A background refetch (triggered by the window-focus manager or the 30s task poll) could land between the optimistic write and the mutation completing, then overwrite the optimistic cache with the still-unarchived server snapshot — making the archived task reappear in the sidebar until the post-mutation invalidate caught up. Match the pattern useDeleteTask already uses: cancelQueries before setQueryData. Also snapshot per-task terminal state and command center cell index before clearing them, and restore both in the error path so a failed archive leaves no orphaned UI state. Generated-By: PostHog Code Task-Id: 3c57db18-63ca-4bc0-9fa5-8fbe8a3795a8
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/renderer/features/tasks/hooks/useArchiveTask.ts:106-108
**`assignTask` unconditionally steals active-task focus during error recovery**
`assignTask` always sets `activeTaskId` to the newly assigned task (see `commandCenterStore.ts` line 113). In the error path this means: if the archived task was visible in the command center but was *not* the active cell, recovery will incorrectly steal focus to it. For example — user focuses cell 0 (task A), right-clicks task B in cell 2 and archives it, mutation fails → `assignTask(2, taskB)` fires and makes task B the active task, silently stealing focus from task A. The snapshot should also capture the pre-archive `activeTaskId` and restore it explicitly instead of relying on `assignTask`'s side-effect.
Reviews (1): Last reviewed commit: "fix(code): prevent archived tasks from r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+106
to
+108
| if (commandCenterIndex !== -1) { | ||
| useCommandCenterStore.getState().assignTask(commandCenterIndex, taskId); | ||
| } |
Contributor
There was a problem hiding this comment.
assignTask unconditionally steals active-task focus during error recovery
assignTask always sets activeTaskId to the newly assigned task (see commandCenterStore.ts line 113). In the error path this means: if the archived task was visible in the command center but was not the active cell, recovery will incorrectly steal focus to it. For example — user focuses cell 0 (task A), right-clicks task B in cell 2 and archives it, mutation fails → assignTask(2, taskB) fires and makes task B the active task, silently stealing focus from task A. The snapshot should also capture the pre-archive activeTaskId and restore it explicitly instead of relying on assignTask's side-effect.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/tasks/hooks/useArchiveTask.ts
Line: 106-108
Comment:
**`assignTask` unconditionally steals active-task focus during error recovery**
`assignTask` always sets `activeTaskId` to the newly assigned task (see `commandCenterStore.ts` line 113). In the error path this means: if the archived task was visible in the command center but was *not* the active cell, recovery will incorrectly steal focus to it. For example — user focuses cell 0 (task A), right-clicks task B in cell 2 and archives it, mutation fails → `assignTask(2, taskB)` fires and makes task B the active task, silently stealing focus from task A. The snapshot should also capture the pre-archive `activeTaskId` and restore it explicitly instead of relying on `assignTask`'s side-effect.
How can I resolve this? If you propose a fix, please make it concise.The previous rollback used assignTask(cellIndex, taskId) to restore the command-center cell on archive failure. assignTask unconditionally sets activeTaskId to the assigned task, so a failed archive of a non-active cell silently stole focus from whatever the user actually had active. Snapshot wasActiveInCommandCenter alongside the cell index, and restore the cell via setState — only writing activeTaskId back when the archived task was the active one. Mirrors removeTaskById's existing semantics. Generated-By: PostHog Code Task-Id: 3c57db18-63ca-4bc0-9fa5-8fbe8a3795a8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Archiving a task in the sidebar sometimes worked cleanly, sometimes silently re-listed the task a moment later, and sometimes appeared to fail outright. The behaviour was timing-dependent.
Root cause:
archiveTaskImperative(apps/code/src/renderer/features/tasks/hooks/useArchiveTask.ts) wrote optimistic updates to the archive queries without first cancelling in-flight refetches. A background refetch (window-focus manager or the 30s task-summaries poll) could land between the optimistic write and the mutation completing, then overwrite the optimistic cache with the still-unarchived server snapshot — making the archived task reappear in the sidebar until the post-mutationinvalidateQueriescaught up.useDeleteTaskalready uses the correctcancelQueries-first pattern; archive did not.Secondary issue: if the archive mutation threw after we'd already called
clearTerminalStatesForTaskandremoveTaskById, the task came back in the sidebar but its terminal state and command-center slot were silently dropped.Changes
await queryClient.cancelQueries(trpc.archive.pathFilter())immediately before the optimisticsetQueryDatacalls inarchiveTaskImperative. This kills any in-flight archive refetch so it can no longer overwrite the optimistic state. The same fix transitively covers the bulk "Archive prior tasks" loop inSidebarMenu, since every iteration callsarchiveTaskImperativeand therefore cancels the previous iteration's invalidate-triggered refetch.taskIdand${taskId}-*) and the command-center cell index before clearing them. On mutation failure, restore both alongside the existing pin/cache rollbacks viauseTerminalStore.setState(...)anduseCommandCenterStore.getState().assignTask(cellIndex, taskId).No new dependencies, no new public API, no behaviour change on the success path beyond eliminating the race.
How did you test this?
pnpm --filter code typecheck— only the pre-existing workspace-package errors remained (@posthog/git/*,@posthog/platform/*); the edited file is clean.npx biome checkon the edited file — passes.biome check --write+pnpm typecheckon staged files) ran on the commit and passed.Publish to changelog?
no